Reviewed 11 of 13 files at r1, all commit messages.
Reviewable status: 11 of 13 files reviewed, 1 unresolved discussion (waiting on @niklasberglund)

ios/MullvadVPNUITests/Pages/AccountPage.swift line 49 at r1 (raw file):

        guard let deviceName = deviceNameLabel.value as? String else {
            XCTFail("Failed to read device name from label")
            return String()

Is returning an empty String a common pattern in our codebase? To me, using an in-band value to represent out-of-band conditions feels like a code smell, and using a String? would be better. Are there ergonomic reasons for doing it this way?

Reviewed 11 of 13 files at r1, all commit messages.
Reviewable status: 11 of 13 files reviewed, 5 unresolved discussions (waiting on @acb-mv and @niklasberglund)

ios/MullvadVPN/View controllers/Settings/SettingsHeaderView.swift line 34 at r1 (raw file):

    let collapseButton: UIButton = {
        let button = UIButton(type: .custom)
        button.accessibilityIdentifier = .expandButton

Why change it to expandButton if its default position is collapsed ?

ios/MullvadVPNUITests/ConnectivityTests.swift line 49 at r1 (raw file):

    }

    /// Get the app into a blocked state by connecting to a relay then applying a filter which don't find this relay, then verify that app can still communicate by logging out and verifying that the device was successfully removed

nit
I'm surprised the comment length didn't trigger any linter violation 🤔

ios/MullvadVPNUITests/ConnectivityTests.swift line 65 at r1 (raw file):

            .tapApplyButton()

        // Select the first country, it's first city and it's first relay

nit
its instead of it's (the city / relay belongs to the parent item)

ios/MullvadVPNUITests/ConnectivityTests.swift line 67 at r1 (raw file):

        // Select the first country, it's first city and it's first relay
        SelectLocationPage(app)
            .tapCountryLocationCellExpandButton(withIndex: 0)

nit
while it's rather unlikely to happen, we should probably have some logic to guarantee we don't select a relay that's deactivated

ios/MullvadVPNUITests/Pages/AccountPage.swift line 49 at r1 (raw file):

@acb-mv I think the return value here is just to satisfy the compiler since we mandate returning a string.
The execution of the test won't continue after this failure.

That being said, we can improve this function like so (and we can even include a custom error message if we want to be explicit about failure)

    func getDeviceName() throws -> String {
        let deviceNameLabel = app.otherElements[AccessibilityIdentifier.accountPageDeviceNameLabel]
        return try XCTUnwrap(deviceNameLabel.value as? String)
    }

Reviewed 2 of 13 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @niklasberglund)

Reviewable status: 11 of 13 files reviewed, 5 unresolved discussions (waiting on @acb-mv and @buggmagnet)

ios/MullvadVPN/View controllers/Settings/SettingsHeaderView.swift line 34 at r1 (raw file):

If it's in a collapsed state then the button will expand its content

ios/MullvadVPNUITests/ConnectivityTests.swift line 49 at r1 (raw file):

Me too!

ios/MullvadVPNUITests/ConnectivityTests.swift line 65 at r1 (raw file):

Haha, to this day I thought "its" and "it's" was the other way around so I've been using them the wrong way most of the time 😄

ios/MullvadVPNUITests/ConnectivityTests.swift line 67 at r1 (raw file):

Yes probably. This applies to other tests also. Been intentionally ignoring this flaw but maybe shouldn't for too long.

ios/MullvadVPNUITests/Pages/AccountPage.swift line 49 at r1 (raw file):

Yes exactly - don't want to return an optional here because it adds complexity without improving anything because the test has already failed and we're not trying to recover from being unable to read device name. I really should be using XCTUnrwap more. It's a good improvement, have implemented it 👍

Reviewable status: 11 of 13 files reviewed, 5 unresolved discussions (waiting on @acb-mv and @buggmagnet)

ios/MullvadVPNUITests/ConnectivityTests.swift line 67 at r1 (raw file):

Created a Linear ticket for this https://linear.app/mullvad/issue/IOS-654/handle-servers-being-down-in-end-to-end-tests

Reviewed 1 of 13 files at r1, 2 of 2 files at r2, 6 of 6 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @acb-mv)

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @acb-mv)

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

Reviewable status: complete! all files reviewed, all discussions resolved